Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update and rename nex-split-tracker to osrs-splits-the-kodai #7211

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

KeyboardIsMagic
Copy link
Contributor

Integrated the features of osrs-splits proposed plugin to the already existing plugin. Changed name to reflect merge.

Integrated the features of osrs-splits proposed plugin to the already existing plugin. Changed name to reflect merge.
@runelite-github-app
Copy link

runelite-github-app bot commented Jan 6, 2025

@tylerwgrass
Copy link
Member

Instead of deleting the old plugin file and creating a new one you can instead just replace the repo and hash in plugins/nex-split-tracker

@KeyboardIsMagic
Copy link
Contributor Author

I didn't think I deleted the old plugin file, I did overwrite that same repo with the new commit which did replace a lot of files and restructured the repo. I made a new fork, changed the name, and updated the url/hash. I'm sorry for any complications this made.

@YvesW
Copy link
Member

YvesW commented Jan 6, 2025

So generally you want to...

  1. Not rename the pluginfile in this repo
  2. Add the new config options to your config (and keep the old one), can potentially add sections.
    This way it's very easy to combine your old and new code. Functions can just be disabled via the config.
  3. Keep the old configgroup (or write migration code that runs in startup and profilechanged IIRC but there's no reason to do that when the old group works).
  4. You can change the name of Plugin.java but IIRC you need to define the old name so it remembers it's on/off state. You can check at what I did with the TimersPlugin when I renamed that (Team plugin has the same IIRC).
  5. If you add new functionality that does http requests (not to github/runelite.net/osrs.game/osrs wiki) and the plugin did not have a warning before or the warning does not encompass all new http functionality, you want to put the http functionality behind a config option that's disabled by default and has a warning attached to it.

@YvesW YvesW added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 6, 2025
@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 6, 2025
@KeyboardIsMagic
Copy link
Contributor Author

KeyboardIsMagic commented Jan 6, 2025

-- changed the name osrs-splits-the-kodai back to original -> nex-split-tracker.

-- changed the config group back to original "nexsplittracker"

-- added ConfigItem enableExternalSharing with an informational warning and wrapped all Http requests around this check.

PluginDescriptor name remains "OSRS Splits - The Kodai" as I believe that is a better name for the plugin, I have not yet looked into YvesW suggestion "You can check at what I did with the TimersPlugin when I renamed that (Team plugin has the same IIRC)." Which I will do now and either update the commit or come back here with questions. Thanks for the patience.

@YvesW YvesW added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 6, 2025
@KeyboardIsMagic
Copy link
Contributor Author

KeyboardIsMagic commented Jan 6, 2025

Could you link me the suggested plugin for reference? I look at your repos, and the plugin-hub for "time" and couldn't find the reference you mentioned. Or if I can get away with directly renaming PluginDescriptor that would be tight.

@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 6, 2025
@Nightfirecat
Copy link
Member

He's talking about the change he made to the core Timers & Buffs plugin (formerly it was the Timers plugin): runelite/runelite@06de8a6

@YvesW
Copy link
Member

YvesW commented Jan 6, 2025

I meant the commit NFC linked above indeed! Mostly aiming at configName = "TimersPlugin",

@YvesW YvesW added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 6, 2025
@KeyboardIsMagic
Copy link
Contributor Author

I understand, thank you.

@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 6, 2025
@KeyboardIsMagic
Copy link
Contributor Author

KeyboardIsMagic commented Jan 12, 2025

I've made those changes, Is there anything else I need to do on my end?
Not sure if I am waiting for review or something else.

@LlemonDuck
Copy link
Contributor

LlemonDuck commented Jan 25, 2025

replace your e.printStackTrace()s with logging equivalents, but also consider whether you need to log them so aggressively anyway since there are likely to be a lot of them as players join/leave/crash out of parties. Can you handle them more gracefully than just dumping out to the log?

@LlemonDuck LlemonDuck added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 25, 2025
@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 28, 2025
@KeyboardIsMagic
Copy link
Contributor Author

-- Removed e.printStackTrace() instances
-- Replaced with proper logging
-- Ignored catch exceptions in irrelevant or frequent cases

@LlemonDuck
Copy link
Contributor

Images that aren't used in the features of your plugin (i.e. the ones used in your readme) shouldn't be in your plugin resources, otherwise they'll be packed in the plugin jar and bloat its size.

You should move them to another folder in the repo and reference them from there.

@LlemonDuck LlemonDuck added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 28, 2025
@KeyboardIsMagic
Copy link
Contributor Author

Images that aren't used in the features of your plugin (i.e. the ones used in your readme) shouldn't be in your plugin resources, otherwise they'll be packed in the plugin jar and bloat its size.

You should move them to another folder in the repo and reference them from there.

Does it matter what level they are in the directory? In your opinion should they be placed inside main or inside java if I'm making a new folder specifically for these images.

@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 28, 2025
@LlemonDuck
Copy link
Contributor

They shouldn't be under src/main/resources particularly, but ideally they're just not in src.

@LlemonDuck LlemonDuck added the waiting for author waiting for the pr author to make changes or respond to questions label Jan 28, 2025
@runelite-github-app runelite-github-app bot removed the waiting for author waiting for the pr author to make changes or respond to questions label Jan 29, 2025
@KeyboardIsMagic
Copy link
Contributor Author

-- Moved all README images to a separate folder located in the root of the repo.

@LlemonDuck
Copy link
Contributor

Change this to use warning = instead of description https://github.com/KeyboardIsMagic/OsrsSplitsTheKodai/blob/master/src/main/java/com/osrs_splits/OsrsSplitsConfig.java#L52

Can I ask why you're reimplementing your own websocket i/o for this instead of using the RL party system? it adds a lot of complexity to this plugin and makes the review that much more time consuming.

@KeyboardIsMagic
Copy link
Contributor Author

If the RL party system could also send custom data to an external server and I had know that at the beginning I would of used that. I built the websocket i/o specifically to handle the use cases of this plugin as requirements from the RuneWatch community to help in scamming prevention and evidence for RW cases.

Sorry for any trouble this plugin as causes and the time sink that goes into its review, myself and the involved communities really appreciate the effort that everyone who has helped with this PR. They are really excited to use this plugin and implement it into their communities.

@LlemonDuck
Copy link
Contributor

It can't send things to external servers so if that's your use case and you're doing more complicated things on the server side that's fine I guess. If you're not doing any server-side processing I'd recommend switching to RL party and just using API requests to your server though.

Thanks for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants